Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement RFC 3184 - thread local cell methods #92123

Merged
merged 7 commits into from
Mar 5, 2022

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Dec 20, 2021

This implements RFC 3184, with @danielhenrymantilla's suggestion for the with_ method names.

Tracking issue: #92122

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2021
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Note that I got a bit confused by the nested Option dance at the beginning, hence some extra comments and renames, but nothing paramount.

library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Show resolved Hide resolved
library/std/src/thread/local.rs Show resolved Hide resolved
library/std/src/thread/local.rs Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 20, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Dec 20, 2021

@danielhenrymantilla Thanks for the review!

@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 5, 2022

This is bocked on an issue with the CI/test runner, where a large output from rustc gets trimmed and <<<<<< SKIPPED 246746 BYTES >>>>>> injected, which results in invalid json, breaking things. See the failed test run: https://github.com/rust-lang/rust/runs/4586149338?check_suite_focus=true

@JohnCSimon JohnCSimon added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jan 23, 2022
@apiraino
Copy link
Contributor

apiraino commented Feb 24, 2022

@m-ou-se checking in for this PR: is the CI issue you mention being taken care in some place? thanks.

If it's fine, I'd temporarily remove the s-waiting-on-review label until the CI issue is solved.

@rustbot label -S-waiting-on-review

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 24, 2022
@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 24, 2022

is the CI issue you mention being taken care in some place?

I filed an issue for it: #94322

Thanks for the reminder.

@m-ou-se m-ou-se force-pushed the thread-local-cell-methods branch from 50e6b83 to 363833b Compare February 24, 2022 14:49
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
…etrochenkov

Avoid emitting full macro body into JSON errors

While investigating rust-lang#94322, it was noted that currently the JSON diagnostics for macro backtraces include the full def_site span -- the whole macro body.

It seems like this shouldn't be necessary, so this PR adjusts the span to just be the "guessed head", typically the macro name. It doesn't look like we keep enough information to synthesize a nicer span here at this time.

Atop rust-lang#92123, this reduces output for the src/test/ui/suggestions/missing-lifetime-specifier.rs test from 660 KB to 156 KB locally.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
…etrochenkov

Avoid emitting full macro body into JSON errors

While investigating rust-lang#94322, it was noted that currently the JSON diagnostics for macro backtraces include the full def_site span -- the whole macro body.

It seems like this shouldn't be necessary, so this PR adjusts the span to just be the "guessed head", typically the macro name. It doesn't look like we keep enough information to synthesize a nicer span here at this time.

Atop rust-lang#92123, this reduces output for the src/test/ui/suggestions/missing-lifetime-specifier.rs test from 660 KB to 156 KB locally.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
…etrochenkov

Avoid emitting full macro body into JSON errors

While investigating rust-lang#94322, it was noted that currently the JSON diagnostics for macro backtraces include the full def_site span -- the whole macro body.

It seems like this shouldn't be necessary, so this PR adjusts the span to just be the "guessed head", typically the macro name. It doesn't look like we keep enough information to synthesize a nicer span here at this time.

Atop rust-lang#92123, this reduces output for the src/test/ui/suggestions/missing-lifetime-specifier.rs test from 660 KB to 156 KB locally.
@bors
Copy link
Contributor

bors commented Feb 27, 2022

☔ The latest upstream changes (presumably #94373) made this pull request unmergeable. Please resolve the merge conflicts.

@m-ou-se m-ou-se force-pushed the thread-local-cell-methods branch from 363833b to 41248d5 Compare February 28, 2022 11:08
@rust-log-analyzer

This comment has been minimized.

@m-ou-se m-ou-se force-pushed the thread-local-cell-methods branch from 41248d5 to e602bef Compare February 28, 2022 15:14
@m-ou-se m-ou-se added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 2, 2022
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 2, 2022

@joshtriplett The CI issue has been fixed, so this is now ready for review.

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 5, 2022

📌 Commit e602beff3293960273780440b63240be5568441c has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2022
@m-ou-se m-ou-se force-pushed the thread-local-cell-methods branch from e602bef to a6e7f26 Compare March 5, 2022 10:39
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 5, 2022

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Mar 5, 2022

📌 Commit a6e7f26 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2022
@bors
Copy link
Contributor

bors commented Mar 5, 2022

⌛ Testing commit a6e7f26 with merge 35d2d53a0ecf89712d3378790151a186fe4f6379...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)



The actual stderr differed from the expected stderr.
Actual stderr saved to /tmp/compiletestTtXhmV/available-concurrency.stage-id.stderr
To only update this specific test, also pass `--test-args available-concurrency.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools-bin/miri" "tests/run-pass/available-concurrency.rs" "-L" "/tmp/compiletestTtXhmV" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/tmp/compiletestTtXhmV/available-concurrency.stage-id" "-A" "unused" "--edition" "2018" "-Astable-features" "--sysroot" "/home/user/.cache/miri/HOST" "-L" "/tmp/compiletestTtXhmV/available-concurrency.stage-id.aux"
------------------------------------------

------------------------------------------
stderr:
---
......... (50/60)
.......... (60/60)


/checkout/src/test/rustdoc-gui/toggled-open-implementations.goml An exception occured: Failed to launch the browser process!
Inconsistency detected by ld.so: dl-tls.c: 493: _dl_allocate_tls_init: Assertion `listp->slotinfo[cnt].gen <= GL(dl_tls_generation)' failed!


TROUBLESHOOTING: https://github.com/puppeteer/puppeteer/blob/master/docs/troubleshooting.md
== STACKTRACE ==
Error
Error
    at innerRunTestCode (/node-v14.4.0-linux-x64/lib/node_modules/browser-ui-test/src/index.js:468:16)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

@bors
Copy link
Contributor

bors commented Mar 5, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 5, 2022
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 5, 2022

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2022
@bors
Copy link
Contributor

bors commented Mar 5, 2022

⌛ Testing commit a6e7f26 with merge ab2bd41...

@bors
Copy link
Contributor

bors commented Mar 5, 2022

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing ab2bd41 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2022
@bors bors merged commit ab2bd41 into rust-lang:master Mar 5, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 5, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ab2bd41): comparison url.

Summary: This benchmark run did not return any relevant results. 37 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@m-ou-se m-ou-se deleted the thread-local-cell-methods branch March 6, 2022 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants